-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support of manual aut_methods for SSH2 connection #173
base: master
Are you sure you want to change the base?
Conversation
|
||
self.protocol.set_auth_methods(['password', 'publickey']) | ||
self.assertTrue(self.protocol.get_auth_methods() is not None) | ||
self.assertListEqual(self.protocol.get_auth_methods(), ['password', 'publickey']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (89 > 79 characters)
Defines the SSH2 list of authentication methods allowed | ||
|
||
:type methods: list | ||
:param methods: A list of authentication methods (check Exscript.protocols.ssh2.auth_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (98 > 79 characters)
@@ -252,6 +253,9 @@ def __init__(self, | |||
:keyword banner_timeout: The time to wait for the banner. | |||
:type encoding: str | |||
:keyword encoding: The encoding of data received from the remote host. | |||
:type auth_methods: list | |||
:keyword auth_methods: The SSH authentication method to process (default to all supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (97 > 79 characters)
Anyone would be able to check this pull request, please ? |
@@ -269,6 +269,8 @@ def _paramiko_auth_autokey(self, username, password): | |||
|
|||
def _get_auth_methods(self, allowed_types): | |||
auth_methods = [] | |||
if self.auth_methods: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to do something like
auth_method_handlers = []
if self.auth_methods:
auth_methods = [m for m in self.auth_methods if m in allowed_types]
else:
auth_methods = allowed_types
for method in auth_methods:
for type_name in auth_types[method]:
auth_method_handlers.append(getattr(self, type_name))
return auth_method_handlers
Otherwise Exscript wouldn try to authenticate using unsupported methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. Thanks for the review - I will change the code.
@@ -282,6 +286,8 @@ def __init__(self, | |||
self.banner_timeout = banner_timeout | |||
self.encoding = encoding | |||
self.send_data = None | |||
self.auth_methods = auth_methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do a sanity check here, i.E.:
for method in auth_methods:
if method not in auth_types:
raise ValueError('unsupported auth_method: ' + repr(method))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this check in the beginning. However the auth_type dict is in the ssh2.py module while this should be done in module protocol.py. If we import ssh2 in protocol we will end up with a circular import.
Another solution would be to have the auth_method attribute set directly inside ssh2 and not in protocol
Which one would you prefer to choose ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave "self.auth_methods = auth_methods" in the protocol adapter, but only check the sanity in the SSH2 adapter. That would allow for code that can be interchanged between Telnet and SSH.
Sorry for the long delay for this pull request. I will try to work on this ASAP |
Regarding discussion in issue #172